-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Remove Entities and EntitySources #375
Conversation
…actor catalogdsource into variable source, modify existing variablesources to use variables, move catalogd variablesource into nestedvariablesource Signed-off-by: dtfranz <dfranz@redhat.com>
Will fix #347 |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress so far. Left a few comments.
func(inputVariableSource input.VariableSource) (input.VariableSource, error) { | ||
return variablesources.NewCatalogdVariableSource(cl), nil | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might have to go higher in the slice. Each variable source in this slice nests the one above and some of the variable sources depend on variables received from the nested sources.
E.g. I believe NewBundlesAndDepsVariableSource
depends on variables produced by catalog variable source.
type CatalogdVariable struct { | ||
*input.SimpleVariable | ||
dependencies []*BundleVariable | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we use this one? Can't find a reference.
@@ -44,12 +64,12 @@ func (es *CatalogdEntitySource) Filter(ctx context.Context, filter input.Predica | |||
return resultSet, nil | |||
} | |||
|
|||
func (es *CatalogdEntitySource) GroupBy(ctx context.Context, fn input.GroupByFunction) (input.EntityListMap, error) { | |||
func (es *CatalogdVariableSource) GroupBy(ctx context.Context, fn input.GroupByFunction) (map[string][]deppy.Variable, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can remove Get
, GroupBy
and Iterate
easily because we do not use them.
Filter
has to be replaced somehow. It has a convenient API of accepting reusable predicate functions.
We might want to have some sort of filter
package to help us work with variables. But I think it is reasonable to start without it and see whether we actually need it (e.g. do we have any repeating patterns?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been playing a bit with filtering & separate structures for catalog data. Here is what I came up with so far: #393
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about it more, I think #393 might not work well with filtering like "is this bundle replaces bundle X?" since in #393 we can filter either bundles or channels or packages at the time. And bundle itself doesn't contain information on channel membership.
Still thinking about how to structure data and/or code best to be able to do what we already doing in codebase.
// these properties are lazy loaded as they are requested | ||
bundlePackage *property.Package | ||
providedGVKs []GVK | ||
requiredGVKs []GVKRequired | ||
requiredPackages []PackageRequired | ||
channel *property.Channel | ||
channelEntry *ChannelEntry | ||
semVersion *bsemver.Version | ||
bundlePath string | ||
mediaType string | ||
mu sync.RWMutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should separate bundle variable (meant for deppy resolution) from bundle data struct holding data from catalog.
Let's close this. We have a new branch where we took a different approch. A PR will be created soon. |
Description
WIP PR for collaboration on the removal of Entities and EntitySources.
Reviewer Checklist